-
Notifications
You must be signed in to change notification settings - Fork 30.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
7.x backport: crypto: support OPENSSL_CONF again (and its dependencies) #11345
7.x backport: crypto: support OPENSSL_CONF again (and its dependencies) #11345
Conversation
/to @italoacasas @bnoordhuis |
thanks @sam-github, I would like to fast-track this backport, potentially landing this tomorrow that way we can include this in the RC. Thoughts? |
Agree on fast-tracking. I didn't think the 48 hour delay applied to backports, but we should get a review by @bnoordhuis soon, or perhaps @jasnell can review these? The reason they didn't cherrypick clean is that #11051 touched a CLI switch that was only introduced in #10116 (which is semver-major). I just removed the part of the commit that touch the switch that doesn't exist in 7.x. |
ac72c3e
to
2f1ce29
Compare
I assume v7.x-staging got rebased, this PR now has 110 commits. Could you rebase @sam-github ? |
This is my bad, I had to force push to remove some commits with specs issues. |
@italoacasas I think that's pretty much unavoidable with the number of commits you're juggling. |
Mutations of the environment can invalidate pointers to environment variables, so make `secure_getenv()` copy them out instead of returning pointers. PR-URL: nodejs#11051 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Move some code around so we can properly test whether the switch actually does anything. PR-URL: nodejs#11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Commit a8734af ("src: make copies of startup environment variables") from two weeks ago introduced a regression in the capturing of the `--icu-data-dir=` switch: it captured the string up to the `=` instead of what comes after it. PR-URL: nodejs#11255 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Allow it to be used anywhere in src/ that env variables with security implications are accessed. PR-URL: nodejs#11006 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
A side-effect of https://github.com/nodejs/node-private/pull/82 was to remove support for OPENSSL_CONF, as well as removing the default read of a configuration file on startup. Partly revert this, allowing OPENSSL_CONF to be used to specify a configuration file to read on startup, but do not read a file by default. If the --openssl-config command line option is provided, its value is used, not the OPENSSL_CONF environment variable. Fix: nodejs#10938 PR-URL: nodejs#11006 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
4f98413
to
a3bf4a2
Compare
rebased btw, will be without internet connection from this afternoon, until Sunday morning also, @italoacasas I don't quite understand why I needed to rebase, the changes all cherry-pick clean I think, based on the fact that the rebase was completely conflict free |
@italoacasas which means I can help with anything for the next 5 hours, but not after |
Moving this to |
Landed in |
@nodejs/lts I request acceptance for 6.x, it isn't meaningful for 4.x, which still respects OPENSSL_CONF It doesn't land clean on 6.x, I am backporting. EDIT: backported: #11583 |
Backport of:
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)